-
-
Notifications
You must be signed in to change notification settings - Fork 25
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Revamp Build Infra for tiny-lru #27
Conversation
.idea/* | ||
node_modules | ||
.idea | ||
*.tgz |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ignore the removal of star, this mainly adds tgz to ignore list.
.travis.yml | ||
Gruntfile.js | ||
benchmark.js | ||
bower.json |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's better to use whitelist as opposed to maintaining whitelist to ignore files during publish. See https://gist.github.com/ceejbot/610773f9990a751db68dc6c66974b8ec#whitelisting-files-with-the-files-array
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't work well IRL, as it'll include a CHANGELOG if present
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nm, it's always included 👍
grunt.registerTask("test", ["eslint", "nodeunit"]); | ||
grunt.registerTask("build", ["copy", "concat"]); | ||
grunt.registerTask("default", ["build", "test", "babili"]); | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is little opinionated. grunt
works fine here, but I feel like it's a lot of config work for such a small library. I mean when the grunt file is larger than the code, we should probably avoid it.
public keys(): string[]; | ||
} | ||
export default Lru; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it seems this was just copied from root to lib, which looks un-necessary. The type file at root works perfectly too if package.json
has types
key.
@@ -1,3 +0,0 @@ | |||
"use strict"; | |||
|
|||
(function (global) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this file is not needed anymore.
} else { | ||
global.lru = factory; | ||
} | ||
}(typeof window !== "undefined" ? window : global)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this file is not needed anymore. the factory function is included in main file src/lru.js
"allowJs": true, | ||
"sourceMap": true | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a minimal typescript
configuration file
} | ||
|
||
return new LRU(max, ttl); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this was moved here from src/outro.js
"grunt-contrib-copy": "^1.0.0", | ||
"grunt-contrib-nodeunit": "^2.0.0", | ||
"grunt-contrib-watch": "^1.1.0", | ||
"grunt-eslint": "^22.0.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove all babel
and grunt
related dependencies.
add nodeunit
and tslib-cli
as dependencies.
"browser": "lib/tiny-lru.js", | ||
"main": "lib/tiny-lru.cjs.js", | ||
"module": "lib/tiny-lru.esm.js", | ||
"types": "tiny-lru.d.ts", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so essentially
-
we want nodejs code
main
property to readlib/tiny-lru.cjs.js
which waslib/tiny-lru.js
before. -
we are changing
types
to just point at file at root and avoid copy to lib. -
we are also renaming
module
entry point fromlib/tiny-lru.es6.js
tolib/tiny-lru.esm.js
-- This is to avoid versioning confusion in future. Releasing es2020 code for example won't need to change this file again in future. -
we are introducing a new
browser
property to publish our modules asumd
-- read here about browser property -
we are also removing un-minified version (we minify by default). I am planning to release a new version of
tslib-cli
which will start producing sourcemaps too, which is not available for now.
Looks good to me. |
What would you categorize this as? major or minor semver change? i'm thinking minor. |
Considering that we are changing output file names, it might impact someone relying on file paths for imports. Specifically the who were following previous webpack usage guidelines in readme. That said I would consider this as a major version |
👍 |
I'm totally gonna copy/pasta this around some of my other repos. |
I have WIP docs here: https://tslib-cli.js.org/ in case you need to refer back. Feel free to let me know if you ever need details on specific parts of |
I feel like this PR might be controversial as its attempting to change lot of things. But I will try to present this as best as I can.
I started looking at tiny-lru as it is a dependency of
graphql-hooks-memcache
while investigating #366. It seemsgraphql-hooks-memcache
is trying to usetiny-lru
as es6 dependency, and es6 exports fromtiny-lru
isn't exactly consumable.Incidentally, I recently started working on tslib-cli which attempts to streamline tooling/setup required for publishing libraries to NPM. In short, it provides:
With these in mind, I started porting
tiny-lru
to usetslib-cli
for build tooling. This PR presents the list of changes needed for such port.Looking into it more, we can even extend this PR to auto generate type definitions, which will need us to port code from
js
tots
. It might also need some work oneslint
totslint
migration if we decide to port code tots
.NB: I will use comments on diff to explain some of the changes in this PR as this description is getting rather long.
Thanks in advance for looking into this ❤️
Output of
npm pack
after these changes:output file sizes after these changes: